Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

postgresql: use team #352905

Merged
merged 2 commits into from
Nov 4, 2024
Merged

postgresql: use team #352905

merged 2 commits into from
Nov 4, 2024

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 1, 2024

Main motivation for this is that I'd like to get a feature-freeze ping: we have old stuff to remove and quite a bit of things ongoing here, so explicitly being part of the check-up process seems like a good thing.

Also added myself and wolfgangwalther to it.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Ma27 Ma27 requested a review from wolfgangwalther November 1, 2024 15:54
@nix-owners nix-owners bot requested a review from thoughtpolice November 1, 2024 15:55
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for taking that up. I wanted to add to the new OWNERS file anyway, but didn't get around to that, yet. Didn't know there was a postgres team already, cool!

Happy to be in it, ofc.

maintainers/team-list.nix Show resolved Hide resolved
@@ -320,7 +320,7 @@ let
description = "Powerful, open source object-relational database system";
license = licenses.postgresql;
changelog = "https://www.postgresql.org/docs/release/${finalAttrs.version}/";
maintainers = with maintainers; [ thoughtpolice danbst globin ivan ma27 wolfgangwalther ];
maintainers = with maintainers; [ danbst globin ivan ] ++ teams.postgres.members;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also take the chance to discuss / ask about the other maintainers listed here. My experience over this year was, that nobody else except @Ma27 ever commented on any of the related Pull Requests I made - which is absolutely fine. In the beginning it was just confusing to me, because the package looked well maintained from looking at the list of maintainers - but it turned out not to be true.

Looking at activity in nixpkgs in general and at postgresql specifically, I think we should:

  • Remove @danbst from the list of maintainers, last active in nixpkgs in ~2020 (pull requests, commits).
  • Check-in with @globin, who committed to nixpkgs as of July this year, but apparently never had any PRs related to PostgreSQL. @globin do you still want to be a maintainer for PostgreSQL? Seems like this could be a case of "I'd like to be notified about changes", in which case I'd suggest we add a comment accordingly, so that people looking at the list can tell they wouldn't need to expect an answer / review here?
  • Check-in with @ivan who recently removed himself from postgresqlPackages.pg_embedding in prisma-engines, postgresqlPackages.pg_embedding: remove myself from maintainers #324978. @ivan did you just forget to remove yourself here or would you like to remain maintainer for postgresql?
  • Check-in with @thoughtpolice - you still seem to be very active on nixpkgs, but haven't heard anything from you in the context of postgresql. Would you (still) like to be part of the postgresql team?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still do use postgres-with-nix behind the scenes and even did some commercial work on it last year, but mostly it's just because I'm spread very thin and don't have time to give it singular focus and many of the PRs require much more attention than I have available.

I'm happy to remain on the maintainers list as a point-of-contact for questions though.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 2, 2024
@Ma27
Copy link
Member Author

Ma27 commented Nov 2, 2024

@wolfgangwalther just created a team for postgres with all members in this branch.

What do we do with the other maintainers? It's not super-urgent to merge it now, but I'd like to not stall this because we're awaiting feedback.

@nix-owners nix-owners bot requested a review from philiptaron November 2, 2024 14:38
/pkgs/servers/sql/postgresql @NixOS/postgres
/nixos/modules/services/databases/postgresql.md @NixOS/postgres
/nixos/modules/services/databases/postgresql.nix @NixOS/postgres
/nixos/tests/postgresql.nix @NixOS/postgres
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we want to move all postgresql-related tests into nixos/tests/postgresql? That way, we could also add the subtree here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with #352966 open, I think it should be fine to move those files here. A rebase should be simple, because git should detect the renames.

@wolfgangwalther
Copy link
Contributor

What do we do with the other maintainers? It's not super-urgent to merge it now, but I'd like to not stall this because we're awaiting feedback.

Let's remove danbst as suggestd above in this PR, then merge. We'll still wait for feedback here and if we don't get any we will remove the others later?

@Ma27
Copy link
Member Author

Ma27 commented Nov 2, 2024

Done.
Let's perform the restructuring of the tests in #352966 if that's OK for you.

@wolfgangwalther
Copy link
Contributor

Let's perform the restructuring of the tests in #352966 if that's OK for you.

👍

Ma27 added 2 commits November 2, 2024 15:47
Main motivation for this is that I'd like to get a feature-freeze ping:
we have old stuff to remove and quite a bit of things ongoing here, so
explicitly being part of the check-up process seems like a good thing.

Also added myself and wolfgangwalther to it.
Thanks a lot for all your work! If you ever come back to nixpkgs, feel
free to revert the commit, your return would be welcomed!
@Ma27
Copy link
Member Author

Ma27 commented Nov 2, 2024

So, the team exists, I now need somebody to add nixpkgs in read-only mode to be added to the team:

You do not have sufficient permissions to add the nixpkgs repository to this team

cc @Lassulus @zimbatm can one of you add nixpkgs (role: readonly) to https://github.com/orgs/NixOS/teams/postgres ?

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Nov 2, 2024
It will be EOL in about a week[1] and should've never reached 24.11.
However, I messed up and missed the freeze deadline, so we can't do
breaking stuff like this, hence we'll mark it as insecure and right
after branchoff it will be removed from unstable.

During that discussion I also got the feedback that it's easy for people
who just do `services.postgresql.enable = true;` to miss their version
getting EOL since there's no warning by the selection logic based on the
state version. Also added that. It's kinda noisy, but I expect it to be
pretty effective for people who are prone to miss the EOL otherwise.

For 25.11 I'd like to make sure we remove postgresql_13 before. To make
it harder for us to miss the deadline, the postgres team will receive a
ping before feature freeze[2].

[1] https://endoflife.date/postgresql
[2] Implemented in NixOS#352905
@Ma27 Ma27 mentioned this pull request Nov 2, 2024
13 tasks
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 2, 2024
@wolfgangwalther
Copy link
Contributor

While you're on this, could you remove me from postgis and pg_safeupdate as maintainer together with the addition of the team? We only added myself, so that I would be notified about potential breaking changes there, but with the postgres team in ci/OWNERS, I will be notified anyway.

@wolfgangwalther
Copy link
Contributor

@Ma27 I see a 👍 by lassulus on:

can one of you add nixpkgs (role: readonly) to https://github.com/orgs/NixOS/teams/postgres

Can you trigger CI again?

@philiptaron philiptaron merged commit 08645b4 into NixOS:master Nov 4, 2024
27 of 28 checks passed
@Ma27 Ma27 deleted the postgresql-team branch November 6, 2024 16:19
wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants